[X86] Check if signed value is too large for fixup under some conditions #150976
Conversation
Fixes llvm#116899. Consistent with other architectures, we don't perform any additional range checks (other than the existing assert) on generic fixup types, as this will break anything that emits a signed value for a generic fixup type, for example (as can happen when setting a symbol to a negative value). Instead, we only check architecture-specific fixup types, which in the case of x86 are always signed. This is slightly different to the previous logic as it also uses the fixup type to determine whether to perform the check, rather than just the PC-relative flag.
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-mc Author: circuit10 (Heath123) ChangesFixes #116899. Consistent with other architectures, we don't perform any additional range checks (other than the existing assert) on generic fixup types, as this will break anything that emits a signed value for a generic fixup type, for example (as can happen when setting a symbol to a negative value). Instead, we only check architecture-specific fixup types, which in the case of x86 are always signed. This is slightly different to the previous logic as it also uses the fixup type to determine whether to perform the check, rather than just the PC-relative flag. Full diff: https://github.com/llvm/llvm-project/pull/150976.diff 2 Files Affected:
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp b/llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
index e213923ccf38e..bb0722351c4dd 100644
--- a/llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
+++ b/llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
@@ -21,6 +21,7 @@
#include "llvm/MC/MCELFObjectWriter.h"
#include "llvm/MC/MCELFStreamer.h"
#include "llvm/MC/MCExpr.h"
+#include "llvm/MC/MCFixup.h"
#include "llvm/MC/MCInst.h"
#include "llvm/MC/MCInstrInfo.h"
#include "llvm/MC/MCObjectStreamer.h"
@@ -721,15 +722,56 @@ void X86AsmBackend::applyFixup(const MCFragment &F, const MCFixup &Fixup,
assert(Fixup.getOffset() + Size <= Data.size() && "Invalid fixup offset!");
int64_t SignedValue = static_cast<int64_t>(Value);
- if (IsResolved && Fixup.isPCRel()) {
- // check that PC relative fixup fits into the fixup size.
+
+ // If the fixup is resolved to an absolute value, and is explictly signed
+ // (either by its type or by being marked as PC-relative), we should check
+ // that it fits.
+ // Otherwise, be conservative because some users may rely on overflow, or
+ // being able to use a signed value where an unsigned value would normally be
+ // expected, so strict checks may break things.
+ bool CheckSignedVal = false;
+ if (IsResolved) {
+ switch (Fixup.getKind()) {
+ default:
+ llvm_unreachable("invalid fixup kind!");
+ case FK_NONE:
+ case FK_Data_1:
+ case FK_Data_2:
+ case FK_Data_4:
+ case FK_Data_8:
+ case FK_SecRel_1:
+ case FK_SecRel_2:
+ case FK_SecRel_4:
+ case FK_SecRel_8:
+ CheckSignedVal = false;
+ break;
+ case X86::reloc_riprel_4byte:
+ case X86::reloc_riprel_4byte_relax:
+ case X86::reloc_riprel_4byte_relax_rex:
+ case X86::reloc_riprel_4byte_relax_rex2:
+ case X86::reloc_riprel_4byte_movq_load:
+ case X86::reloc_riprel_4byte_movq_load_rex2:
+ case X86::reloc_riprel_4byte_relax_evex:
+ case X86::reloc_signed_4byte:
+ case X86::reloc_signed_4byte_relax:
+ case X86::reloc_global_offset_table:
+ case X86::reloc_branch_4byte_pcrel:
+ CheckSignedVal = true;
+ break;
+ }
+
+ if (Fixup.isPCRel())
+ CheckSignedVal = true;
+ }
+
+ if (CheckSignedVal) {
if (Size > 0 && !isIntN(Size * 8, SignedValue))
getContext().reportError(Fixup.getLoc(),
"value of " + Twine(SignedValue) +
" is too large for field of " + Twine(Size) +
((Size == 1) ? " byte." : " bytes."));
} else {
- // Check that uppper bits are either all zeros or all ones.
+ // Check that upper bits are either all zeros or all ones.
// Specifically ignore overflow/underflow as long as the leakage is
// limited to the lower bits. This is to remain compatible with
// other assemblers.
diff --git a/llvm/test/MC/X86/fixup-range-check.s b/llvm/test/MC/X86/fixup-range-check.s
new file mode 100644
index 0000000000000..7586ed528d7f8
--- /dev/null
+++ b/llvm/test/MC/X86/fixup-range-check.s
@@ -0,0 +1,8 @@
+// RUN: not llvm-mc -filetype=obj -o /dev/null -triple x86_64-unknown-unknown %s 2>&1 | FileCheck %s
+
+.intel_syntax noprefix
+.code64
+test_case:
+// CHECK: error: value of -9223372036854775808 is too large for field of 4 bytes.
+ mov rcx, EXAMPLE
+.set EXAMPLE, (1ULL<<63ULL)
|
Heath123
changed the title
| if (Fixup.isPCRel()) | ||
| CheckSignedVal = true; |
There was a problem hiding this comment.
Why checking isPCRel since we listed all kinds above?
There was a problem hiding this comment.
Sometimes a PC-relative fixup uses one of the generic fixup types. I think this is because all of the architecture-specific ones are 4-byte, so for example a 1-byte PC-relative fixup will be emitted as a generic fixup, but with the PC-relative flag set to true
|
In When |
I did this because in the case that which generates a non-PC-relative fixup with a negative value. So, in this case we don't want to check it as an unsigned value either. While other backends do no checks at all here, x86 did already have an assert on the code path (not triggered in this case) before, so I've kept that behaviour, as I'm trying to make the checks more strict rather than less. Could you clarify what you mean by "fixed"? Should the assert be removed for consistency with other backends, changed to an error, or something else?
Do you mean for non-PC-relative generic fixups? This is difficult because as described above we don't know if the value is signed, and I can't find any other backends that attempt this. Ideally the fixup structure would keep track of whether it is a signed value or not, but this would be a major change and isn't in scope for this PR which aims to just improve the current situation. We could turn the existing loose assert (which allows some overflow) into a proper error check, though. |
|
Ping |
Fixes #116899. Consistent with other architectures, we don't perform any additional range checks (other than the existing assert) on generic fixup types, as this will break anything that emits a signed value for a generic fixup type, for example (as can happen when setting a symbol to a negative value). Instead, we only check architecture-specific fixup types, which in the case of x86 are always signed. This is slightly different to the previous logic as it also uses the fixup type to determine whether to perform the check, rather than just the PC-relative flag.